Tests: Run slow scanArtifactsForTokens test in CI only by default#3874
Tests: Run slow scanArtifactsForTokens test in CI only by default#3874henrymercer merged 2 commits intomainfrom
scanArtifactsForTokens test in CI only by default#3874Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the artifact-scanner test suite to avoid running an expensive zip-extraction/token-scan test during typical local runs, while still exercising it in CI to preserve coverage of the regression scenario.
Changes:
- Adds an environment-gated condition so the slow “scanArtifactsForTokens finds token in debug artifacts” test runs by default only in CI.
- Introduces a local opt-in mechanism via
RUN_SLOW_TESTS=1. - Documents the rationale for the gating inline in the test file.
Show a summary per file
| File | Description |
|---|---|
src/artifact-scanner.test.ts |
Gates the slow artifact-scanning regression test to CI by default with a local opt-in environment variable. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Just a question about why this also changes the test to not run on Windows, given that it would only run in CI.
Aside from that, it makes sense to me to only run this in CI.
| // This test is slow (extracts and scans a zip artifact), so by default we only run it in CI. Set | ||
| // RUN_SLOW_TESTS=1 to run it locally. | ||
| if ( | ||
| os.platform() !== "win32" && |
There was a problem hiding this comment.
If we are only running this test in CI, why not also run it on Windows? Given the nature of the function being tested, it seems like we could easily miss a Windows-specific problem.
If there is a good reason why we aren't running it on Windows, it would be good if the comment noted it and explained why.
There was a problem hiding this comment.
The artifact scanner isn't compatible with Windows — it's only used in our debug artifact tests, which always run on Linux. It's not compatible because it directly shells out to extract archives.
There was a problem hiding this comment.
Ah, I had a look at the implementation but didn't see an obvious guard against running on Windows. I saw the isInTestMode() check, but I didn't verify whether we always run on Linux there.
In that case, it might be good to make this more explicit in the main implementation in scanArtifactsForTokens as well and return with a suitable message there if it is somehow running on Windows?
| ); | ||
| } | ||
|
|
||
| if (process.platform === "win32") { |
There was a problem hiding this comment.
Of course it is win32 regardless of architecture.
The "scanArtifactsForTokens finds token in debug artifacts" test takes 4.7 seconds to complete on my machine. Run it in CI only by default.
Here's proof that it actually runs.